Changes to the code generated for builtin derived traits.

cc @nnethercote

@bors try @rust-timer queue

☀️ Try build successful (CI)
Build commit: 5a131e3 (5a131e34e34187b36455f6f6809f9cd14e7ab55f, parent: e9e32aca5a4ffd08cbc29547b039d64b92a2c03b)

Finished benchmarking commit (5a131e3): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

Bootstrap: 492.421s -> 491.503s (-0.19%)
Artifact size: 394.25 MiB -> 394.27 MiB (0.01%)

There was a leftover condition from an earlier PR that I pulled in without a proper review, such that it actually didn't apply the optimization to large enums <_<;

I also want to try skipping bounds checks. I suppose it could improve runtime performance, at least on my machine, based on some pretty rough benchmarks with an enum of 10,000 variants.

@bors try @rust-timer queue

I'm happy to review this once it's ready.

@rustbot author

☀️ Try build successful (CI)
Build commit: 7348f26 (7348f264e27b4bbcab21ed426532865b98fc5f55, parent: 8da2d28cbd5a4e2b93e028e709afe09541671663)

Finished benchmarking commit (7348f26): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

Bootstrap: 493.313s -> 504.472s (2.26%)
Artifact size: 394.36 MiB -> 394.34 MiB (-0.01%)

This will probably run into #148423. Furthermore, I believe this is likely to cause unsoundness in actual programs, due to the enumflags2 crate which I think has a macro that modifies the discriminant values of enums.

Given that it's a lang issue and we basically have no way to work around in this PR...

@rustbot blocked #148423

We still have compile time regression, I suppose that for small enums it might not be worth doing this kind of optimization, I don't think the runtime performance would vary a lot but the compile time has already increased a lot.

☔ The latest upstream changes (presumably #155796) made this pull request unmergeable. Please resolve the merge conflicts.

@makai410 The piece of code I was looking for when we were discussing this at all hands is here: https://github.com/makai410/rust/blob/b16703bbee4b9ec6e790bbb88115c84be293635b/tests/ui/deriving/deriving-all-codegen.stdout#L445, the optimization was introduced in #98190

The derive generates array of names, and array of dyn Debug references to fields and passes that to internal non-generic debug_struct_fields_finish function. There are also specialized functions for smaller structs.

I think what you probably want to do here is very similar - generate just the arrays that are unique to each struct and pass them to some internal function to avoid duplicating a lot of similar code in multiple derives.

Also, with this approach it's maybe not even worth it to concatenate the string at all, and just use list of names and discriminant as an index, but we'd have to measure that.

Also, with this approach it's maybe not even worth it to concatenate the string at all, and just use list of names and discriminant as an index, but we'd have to measure that.

Hi @panstromek, sorry for the delay. The reason for doing concatenated string is that there was a #109615 which tried a names array but it turned out to be not much of an improvement, and the idea of concatenated string was inspired by #133945 (comment), which suggested that the concatenated string could reduce the number of relocations, so I still want to try the concatenated string here.

As for the threshold number of variants, I was planning to write a benchmark tool to measure it. The idea is basically to use several well-known rust projects as compilation targets, add a custom compiler flag (-Zbike-shedding-threshold=100), sweep the threshold from 100 to 200 in steps of 20, measure the runtime improvement and compile-time overhead, and compute their ratio. A higher ratio means we trade less compile-time overhead for more runtime improvement. We can then plot the threshold on the X axis and the ratio on the Y axis, and then the threshold corresponding to the highest ratio is likely what we want, but worth noting here we still have to look at the actual number of the compile-time overhead at the highest ratio point, which might be a large number(?), to check if the number is acceptable. This is just a rough idea that can probably miss something, but well I think the overall direction is correct?

EDIT:
The problem might not be that complicated. I suspect there wouldn't be much difference between 100 and 200, since I don't think there are many enums with that many variants in this range. I suspect even 50 variants is rare in practice... So maybe I should just pick several favorite numbers and run several rounds of perf to see the results :)

I see, I didn't know about #109615, the concatenated string approach makes sense then.

So maybe I should just pick several favorite numbers and run several rounds of perf to see the results

Yea, I'd probably do something lazy like that :D. You can kinda bisect to it. But note that I'm not sure how many C-like enums with derive(Debug) we have in our benchmarks suite - especially big ones - so we might not be able to see the effect without adding a new one (or at least confirming it locally).

btw. Maybe it's also worth trying to pad all variants to the same length and indexing the resulting string by max_length * discriminant, to avoid generating second array. This way the derive would be bit more minimal (at the cost of some wasted space), something like this:

enum Fieldless0 { A, BB, CCC }
//...
const __NAMES: &str = "A  BB CCC";
debug_c_like_enum(
    __names,
   discriminant_value(self),
   3 /* max variant name len */
)